collision_distance_field, port to ROS 2 #65

vmayoral · 2019-03-31T16:47:20Z

No description provided.

vmayoral · 2019-03-31T17:23:00Z

I've left the logger at the collision_distance_field_types.h file which is then imported by all cpp other cpp files. Let me know if you'd prefer an individual instance of a logger per cpp file instead.

vmayoral · 2019-04-08T07:49:34Z

See #62 (comment)

Co-authored-by: vmayoral <[email protected]>

mlautman · 2019-05-29T18:30:46Z

moveit_core/collision_distance_field/CMakeLists.txt


-if(CATKIN_ENABLE_TESTING)
+ament_target_dependencies(${MOVEIT_LIB_NAME}


please fix white space.

AcutronicRobotics@7077bf2#diff-4f4a9784d0b28ca2338112f521150d9e

moveit_core/collision_distance_field/CMakeLists.txt

mlautman · 2019-05-29T18:33:39Z

moveit_core/collision_distance_field/CMakeLists.txt

+	if(WIN32)
+    # TODO add windows paths
+	else()
+		set(append_library_dirs "${CMAKE_CURRENT_BINARY_DIR};${CMAKE_CURRENT_BINARY_DIR}/../planning_scene;${CMAKE_CURRENT_BINARY_DIR}/../distance_field;${CMAKE_CURRENT_BINARY_DIR}/../collision_detection;${CMAKE_CURRENT_BINARY_DIR}/../robot_state;${CMAKE_CURRENT_BINARY_DIR}/../robot_model;${CMAKE_CURRENT_BINARY_DIR}/../utils")


Using hardcoded paths is a bad idea. Is this really necessary?

I would recommend that for every library in this package that is used else ware you set a <lib_name>_lib_dir environment variable that can then be used by the tests

A good example is the rcl_lib_dir I have taken the time to show how it is used below:

/home/mike/ws_ros2/src/ros2/rcl/rcl/CMakeLists.txt: 78 79: # rcl_lib_dir is passed as APPEND_LIBRARY_DIRS for each ament_add_gtest call so 80 # the librcl that they link against is on the library path. 81 # This is especially important on Windows. 82 # This is overwritten each loop, but which one it points to doesn't really matter. 83: set(rcl_lib_dir "$<TARGET_FILE_DIR:${PROJECT_NAME}>") 84 /home/mike/ws_ros2/src/ros2/rcl/rcl/test/CMakeLists.txt: 16 17: set(extra_lib_dirs "${rcl_lib_dir}") 18 /home/mike/ws_ros2/src/ros2/rcl/rcl/test/CMakeLists.txt: 34 rcl_add_custom_gtest(test_client${target_suffix} 35 SRCS rcl/test_client.cpp 36: INCLUDE_DIRS ${osrf_testing_tools_cpp_INCLUDE_DIRS} 37 ENV ${rmw_implementation_env_var} 38 APPEND_LIBRARY_DIRS ${extra_lib_dirs}

The example uses rcl_add_custom_gtest but it works just as well with ament_add_gtest

If you don't mind I prefer to merge this. And then I will fix them all together. Because we have other merged that use this style. I can open an issue to avoid forgetting it.

moveit_core/collision_distance_field/CMakeLists.txt

mlautman · 2019-05-29T18:35:52Z

...sion_distance_field/include/moveit/collision_distance_field/collision_distance_field_types.h

+#include "rclcpp/time.hpp"
+#include "rclcpp/utilities.hpp"
+
+static rclcpp::Logger LOGGER_COLLISION_DISTANCE_FIELD = rclcpp::get_logger("moveit").get_child("collision_distance_field");


place inside namespace.

...sion_distance_field/include/moveit/collision_distance_field/collision_distance_field_types.h

mlautman · 2019-05-29T18:39:25Z

...sion_distance_field/include/moveit/collision_distance_field/collision_robot_distance_field.h

@@ -102,37 +103,37 @@ class CollisionRobotDistanceField : public CollisionRobot
  void checkSelfCollision(const collision_detection::CollisionRequest& req, collision_detection::CollisionResult& res,
                          const moveit::core::RobotState& state1, const moveit::core::RobotState& state2) const override
  {
-    ROS_ERROR_NAMED("collision_distance_field", "Not implemented");
+    RCLCPP_ERROR(LOGGER_COLLISION_DISTANCE_FIELD, "Not implemented");


This class should have it's own logger. Perhaps
static rclcpp::Logger LOGGER_COLLISION_ROBOT_DISTANCE_FIELD = LOGGER_COLLISION_DISTANCE_FIELD.get_child("collision_robot_distance_field")

Changed both, for collision_robot_distance_field and collision_world_distance_field

86c82b3
6fc6693
2449e1e

...sion_distance_field/include/moveit/collision_distance_field/collision_world_distance_field.h

…to withespaces

anasarrak · 2019-06-13T08:29:44Z

Please @mlautman review the changes

mlautman · 2019-06-13T20:21:38Z

...sion_distance_field/include/moveit/collision_distance_field/collision_distance_field_types.h


 namespace collision_detection
 {
+  static rclcpp::Logger LOGGER_COLLISION_DISTANCE_FIELD = rclcpp::get_logger("moveit").get_child("collision_distance_field");


mlautman · 2019-06-13T20:22:52Z

moveit_core/collision_distance_field/src/collision_distance_field_types.cpp

-  ROS_DEBUG_STREAM("BodyDecomposition generated " << collision_spheres_.size() << " collision spheres out of "
-                                                  << shapes.size() << " shapes");
+  RCLCPP_DEBUG(LOGGER_COLLISION_DISTANCE_FIELD, "BodyDecomposition generated %i collision spheres out of %i shapes",
+                    collision_spheres_.size(), shapes.size());


mlautman · 2019-06-13T20:24:36Z

moveit_core/collision_distance_field/src/collision_distance_field_types.cpp

      arrow_mark.header.frame_id = frame_id;
-      arrow_mark.header.stamp = ros::Time::now();
+      arrow_mark.header.stamp = ros_clock.now();


http://docs.ros2.org/bouncy/api/rclcpp/classrclcpp_1_1_clock.html

This needs to be RCL_ROS_TIME not RCL_SYSTEM_TIME which is the default

mlautman · 2019-06-13T20:24:45Z

moveit_core/collision_distance_field/src/collision_distance_field_types.cpp

      sphere_mark.header.frame_id = frame_id;
-      sphere_mark.header.stamp = ros::Time::now();
+      sphere_mark.header.stamp = ros_clock.now();


http://docs.ros2.org/bouncy/api/rclcpp/classrclcpp_1_1_clock.html

This needs to be RCL_ROS_TIME not RCL_SYSTEM_TIME which is the default

mlautman · 2019-06-13T20:25:28Z

moveit_core/collision_distance_field/src/collision_robot_distance_field.cpp

@@ -1065,7 +1075,8 @@ void CollisionRobotDistanceField::addLinkBodyDecompositions(
    link_body_decomposition_vector_.push_back(bd);
    link_body_decomposition_index_map_[link_models[i]->getName()] = link_body_decomposition_vector_.size() - 1;
  }
-  ROS_DEBUG_STREAM(__FUNCTION__ << " Finished ");
+  // RCLCPP_DEBUG_FUNCTION(LOGGER_COLLISION_ROBOT_DISTANCE_FIELD, __FUNCTION__," Finished ");


remove dead code

mlautman · 2019-06-13T20:26:23Z

moveit_core/collision_distance_field/src/collision_world_distance_field.cpp

-  ros::WallTime n = ros::WallTime::now();
-
+  // WallTime
+  auto n = std::chrono::system_clock::now();


http://docs.ros2.org/bouncy/api/rclcpp/classrclcpp_1_1_clock.html

You can use ros::Clock with RCL_SYSTEM_TIME

mlautman · 2019-06-13T20:26:38Z

moveit_core/collision_distance_field/src/collision_world_distance_field.cpp

-  ROS_DEBUG_NAMED("collision_distance_field", "Modifying object %s took %lf s", obj->id_.c_str(),
-                  (ros::WallTime::now() - n).toSec());
+  RCLCPP_DEBUG(LOGGER_COLLISION_WORLD_DISTANCE_FIELD, "Modifying object %s took %lf s", obj->id_.c_str(),
+               std::chrono::system_clock::now() - n);


http://docs.ros2.org/bouncy/api/rclcpp/classrclcpp_1_1_clock.html

Same as above

…TEM_TIME

anasarrak · 2019-06-17T08:24:28Z

@mlautman your suggestion has been addressed, you can review them

anasarrak · 2019-06-21T07:24:49Z

Ping @mlautman

ahcorde · 2019-06-24T07:36:27Z

friendly ping @mlautman @henningkayser @davetcoleman

mlautman · 2019-06-25T21:53:46Z

moveit_core/collision_distance_field/CMakeLists.txt

@@ -1,10 +1,5 @@
 set(MOVEIT_LIB_NAME moveit_collision_distance_field)

-if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")


Why are we removing this. It is only a warning. Rather than remove, we should mark the overloaded functions as virtual and the overloading functions as override

mlautman · 2019-06-25T21:56:04Z

moveit_core/collision_distance_field/CMakeLists.txt

+  if(WIN32)
+    # TODO add windows paths
+  else()
+    set(append_library_dirs "${CMAKE_CURRENT_BINARY_DIR};${CMAKE_CURRENT_BINARY_DIR}/../planning_scene;${CMAKE_CURRENT_BINARY_DIR}/../distance_field;${CMAKE_CURRENT_BINARY_DIR}/../collision_detection;${CMAKE_CURRENT_BINARY_DIR}/../robot_state;${CMAKE_CURRENT_BINARY_DIR}/../robot_model;${CMAKE_CURRENT_BINARY_DIR}/../utils")


Add TODO for fixing this and assign to yourself. Otherwise, we will likely forget to get back to this

Update Dockerfile

vmayoral added 2 commits March 31, 2019 18:46

collision_distance_field, port to ROS 2

a61f85e

collision_distance_filed, adapt logging to moveit#21

6eb216e

Merge branch 'moveit2' into collision-distance-field

39d9a67

ahcorde added 5 commits May 24, 2019 15:38

Fixing collision distance field tests

70e566b

moveit_core collision_distance_field - library as shared

8f25b8f

moveit_core collision_detection_field - adding TODO to CMakeLists

f874662

moveit_core collision_detection_field - fixing logger variables

81a66cc

moveit_core collision_detection_field - fixing CMakeLists.txt

c548aa8

Co-authored-by: vmayoral <[email protected]>

ahcorde mentioned this pull request May 24, 2019

Moveit core CMakeLists.txt #90

Closed

14 tasks

mlautman reviewed May 29, 2019

View reviewed changes

moveit_core/collision_distance_field/CMakeLists.txt Outdated Show resolved Hide resolved

mlautman reviewed May 29, 2019

View reviewed changes

moveit_core/collision_distance_field/CMakeLists.txt Outdated Show resolved Hide resolved

mlautman reviewed May 29, 2019

View reviewed changes

moveit_core/collision_distance_field/CMakeLists.txt Outdated Show resolved Hide resolved

mlautman reviewed May 29, 2019

View reviewed changes

ahcorde and others added 5 commits June 3, 2019 10:37

moveit_core collision_distince_field - CMakeLists.txt convert tabs in…

7077bf2

…to withespaces

Move collision_distance_field_type inside namespace

ad96e04

Update colision_world_distance_field logger

86c82b3

Update collision_robot_distance_field logger

6fc6693

Adding the correct logger name

2449e1e

anasarrak mentioned this pull request Jun 13, 2019

Update moveit core AcutronicRobotics/moveit2#115

Merged

mlautman reviewed Jun 13, 2019

View reviewed changes

anasarrak added 4 commits June 17, 2019 10:18

collision_distance_field_types, Using RCL_ROS_TIME instead of RCL_SYS…

65137db

…TEM_TIME

collision_robot_distance_field, removing dead code

7edf376

collision_world_distance_Field, using RCL_SYSTEM_TIME

7f4b0e5

Clang Format

4cd53f4

mlautman approved these changes Jun 25, 2019

View reviewed changes

mlautman merged commit e29b6cd into moveit:master Jun 25, 2019

anasarrak mentioned this pull request Jun 26, 2019

Update master AcutronicRobotics/moveit2#123

Merged

JafarAbdi pushed a commit to JafarAbdi/moveit2 that referenced this pull request Oct 28, 2019

Merge pull request moveit#65 from AcutronicRobotics/addompl

08cdd3c

Update Dockerfile

MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022

Source setup.bash only once and simplify tutorials (moveit#65)

6b1167b

MikeWrock pushed a commit to MikeWrock/moveit2 that referenced this pull request Aug 15, 2022

Add conditional installation of sphinx to build_locally.sh (moveit#65)

9be187b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

collision_distance_field, port to ROS 2 #65

collision_distance_field, port to ROS 2 #65

vmayoral commented Mar 31, 2019

vmayoral commented Mar 31, 2019

vmayoral commented Apr 8, 2019

mlautman May 29, 2019

anasarrak Jun 13, 2019

mlautman May 29, 2019

mlautman May 29, 2019 •

edited

Loading

ahcorde May 29, 2019

mlautman May 29, 2019

anasarrak Jun 13, 2019

mlautman May 29, 2019

anasarrak Jun 13, 2019

anasarrak commented Jun 13, 2019

mlautman Jun 13, 2019

anasarrak Jun 17, 2019

mlautman Jun 13, 2019

anasarrak Jun 17, 2019

mlautman Jun 13, 2019

anasarrak Jun 17, 2019

mlautman Jun 13, 2019

anasarrak Jun 17, 2019

mlautman Jun 13, 2019

anasarrak Jun 17, 2019

mlautman Jun 13, 2019

anasarrak Jun 17, 2019

mlautman Jun 13, 2019

anasarrak Jun 17, 2019

anasarrak commented Jun 17, 2019

anasarrak commented Jun 21, 2019

ahcorde commented Jun 24, 2019

mlautman Jun 25, 2019

mlautman Jun 25, 2019


		if(CATKIN_ENABLE_TESTING)
		ament_target_dependencies(${MOVEIT_LIB_NAME}

		@@ -1,10 +1,5 @@
		set(MOVEIT_LIB_NAME moveit_collision_distance_field)

		if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang")

collision_distance_field, port to ROS 2 #65

collision_distance_field, port to ROS 2 #65

Conversation

vmayoral commented Mar 31, 2019

vmayoral commented Mar 31, 2019

vmayoral commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlautman May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anasarrak commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anasarrak commented Jun 17, 2019

anasarrak commented Jun 21, 2019

ahcorde commented Jun 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlautman May 29, 2019 •

edited

Loading